fix(recovery): refuse the rollback reset when the snapshot fails (#340) - #341
Conversation
The two preserve steps were asymmetric: preserve_attempt_commits refused to reset past commits it could not park, while a failed snapshot_worktree was journaled and safe_reset ran anyway, destroying the tracked edits and run-created untracked files the snapshot existed to capture. That guarded the more recoverable half — orphaned commits survive in the object store until gc, an uncommitted edit a reset --hard discards does not. preserve_attempt_worktree now takes the same required allow_pause its sibling does and refuses on the same terms, behind a _reset_would_destroy probe so a capture failure over a tree with nothing unparked left to lose still resets rather than halting an unattended run (#123). The probe reuses attempt_dirty against HEAD — commits are already parked or paused on by then — and fails safe on its own git fault (#156). New notice shape (d) names workspace.root, so an in-worktree pause targets the mounted tree rather than the operator's checkout (#161), and offers a git-free rescue: whatever broke the snapshot may still be breaking git. Closes #340
safe_rollback swallowed a failed `git stash create` into an empty snapshot, which disabled the entire preserve restore below it. The hard reset then reverted exactly the paths the caller asked to keep — a resolved re-drive's corrected spec — with no error raised or journalled anywhere, which is the regression the preserve argument exists to prevent. Raise before the reset, scoped to callers that requested a restore: with no preserve the snapshot is unused, so both sweep call sites keep degrading. A clean tree still exits rc 0 with empty output and is unaffected.
_run_git translates only a timeout, so a spawn-level EMFILE/ENOMEM from the snapshot's nine git children escaped as a bare OSError — past every except GitError guard, out of the middle of a rollback, into the crash handler. The sibling guards (is_ancestor, worktree_prune) already catch both. Widen the caller, not the reset: capturing the attempt is observation and may degrade, so it joins the journal-and-decide path; safe_reset is the repair write and still raises.
…sections The first pass created duplicate Changed/Fixed headings inside Unreleased and dropped the Added heading, orphaning the #333 entry. Caught by markdownlint MD024.
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughChangesRollback recovery behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Engine
participant RecoveryFlow
participant Snapshot
participant Operator
Engine->>RecoveryFlow: rollback_or_pause(task, cause)
RecoveryFlow->>Snapshot: preserve attempt worktree
Snapshot-->>RecoveryFlow: success or snapshot failure
RecoveryFlow->>RecoveryFlow: evaluate reset destruction
RecoveryFlow->>Operator: provide rescue instructions
RecoveryFlow-->>Engine: pause or continue re-drive
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_recovery_flow.py (1)
430-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: reuse the helper in the earlier test.
test_snapshot_failure_leaves_a_commits_only_ref_flagged_partial(lines 410-413) still hand-rolls the identical_fail+monkeypatch.setattr. Since_fail_snapshotnow exists, collapsing that one keeps a single definition of "the snapshot blew up".♻️ Proposed cleanup (move `_fail_snapshot` above line 386, then)
- def _fail(*_a, **_k): - raise verify.GitError("simulated commit-tree failure") - - monkeypatch.setattr(verify, "snapshot_worktree", _fail) + _fail_snapshot(monkeypatch)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_recovery_flow.py` around lines 430 - 437, Reuse the existing _fail_snapshot helper in test_snapshot_failure_leaves_a_commits_only_ref_flagged_partial instead of defining another failure callback and monkeypatching verify.snapshot_worktree inline. Move _fail_snapshot earlier if needed so both tests can call the single shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/recovery_flow.py`:
- Around line 460-464: Update the exception handler in the probe flow
surrounding verify.rev_parse_head and verify.attempt_dirty to catch both
verify.GitError and OSError, matching preserve_attempt_worktree and preserving
the fail-safe return True behavior. Also update the related docstring to
explicitly mention OSError as an indeterminate probe failure.
---
Nitpick comments:
In `@tests/test_recovery_flow.py`:
- Around line 430-437: Reuse the existing _fail_snapshot helper in
test_snapshot_failure_leaves_a_commits_only_ref_flagged_partial instead of
defining another failure callback and monkeypatching verify.snapshot_worktree
inline. Move _fail_snapshot earlier if needed so both tests can call the single
shared helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 408685b6-7bef-447e-a322-2551e553a320
📒 Files selected for processing (8)
CHANGELOG.mddocs/FEATURES.mdsrc/bmad_loop/engine.pysrc/bmad_loop/recovery_flow.pysrc/bmad_loop/verify.pytests/test_engine.pytests/test_recovery_flow.pytests/test_verify.py
Greptile SummaryThis PR inverts the contract for
Confidence Score: 5/5Safe to merge; the three code changes are independent, each guarded by an inverse ablation test, and the full suite of 3383 tests is green. The gate-vs-safety-net inversion is narrowly scoped (inert on default config, biting only rollback_on_failure=true and in-worktree dev retries), the _reset_would_destroy probe correctly probes dirty-vs-HEAD rather than dirty-vs-baseline so committed-work-only trees still reset cleanly, and both the OSError-broadening and the stash create raise are scoped tightly to the paths that need them. Previously flagged gaps — OSError uncaught in _reset_would_destroy and in the advisory commits_above probe inside pause_for_manual_recovery — are both resolved in this diff. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/recovery_flow.py | Core of the PR: preserve_attempt_worktree gains allow_pause, _reset_would_destroy probe added, preserve_attempt_commits now guards both git calls, and the advisory probe in pause_for_manual_recovery correctly broadened to catch (GitError, OSError) — all consistently fail-safe. |
| src/bmad_loop/verify.py | safe_rollback now raises on stash create failure when preserve is set (scoped correctly to avoid breaking sweep callers); snapshot_worktree docstring updated to reflect the gate-not-safety-net contract; clean tree path (rc=0, empty output) unaffected. |
| src/bmad_loop/engine.py | Thin forwarding layer updated to pass allow_pause and snapshot_failed through to RecoveryFlow; no logic lives here, just signature parity with the implementation. |
| tests/test_recovery_flow.py | Comprehensive test additions: snapshot-fail pause, no-pause when nothing at risk (inverse ablation), probe fault fails safe, OSError degrades, re-drive never pauses, unit-worktree notice names the right root, notice probe OSError doesn't swallow the pause — all 10 ablations covered per the PR table. |
| tests/test_engine.py | Engine-level end-to-end twin for #340 added; existing test correctly moved to cause="resolved" path since a plain rollback now pauses rather than resets on snapshot failure. |
| tests/test_verify.py | Two new tests for safe_rollback: one confirms it raises before the reset when stash create fails with preserve set, the other (inverse ablation) confirms it degrades when no preserve was requested. |
| docs/FEATURES.md | Documentation updated to describe the new gate-not-safety-net behavior and note that the commits-only defer flag is now reachable only on a re-drive since #340; accurate. |
| CHANGELOG.md | Changelog entries for #340, #343 fixes, and safe_rollback stash-create fix are clear and accurate; blast-radius note (inert under shipped defaults) is correctly stated. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[preserve_attempt_worktree called] --> B{baseline_commit set?}
B -- No --> Z[return no-op]
B -- Yes --> C[snapshot_worktree]
C -- success, parked=None --> D[return tree clean vs HEAD]
C -- success, parked=ref --> E[task.preserve_ref = ref
journal attempt-worktree-preserved]
E --> Z
C -- GitError or OSError --> F[journal attempt-worktree-preserve-failed
task.preserve_partial = True]
F --> G{allow_pause?}
G -- False re-drive --> Z2[return journal-and-proceed
safe_reset runs next]
G -- True plain rollback --> H[_reset_would_destroy probe
rev_parse_head + attempt_dirty vs HEAD]
H -- GitError or OSError --> I[fail-safe: return True]
H -- False tree clean vs HEAD --> J[return
safe_reset runs harmless reset]
H -- True uncommitted work at risk --> K[pause_for_manual_recovery
snapshot_failed=True
always raises RunPaused]
K --> L[Notice shape d:
name working tree,
offer copy-out rescue,
point at journal breadcrumb]
Reviews (6): Last reviewed commit: "fix(recovery): escalate on an OSError fr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Found by CodeRabbit and Greptile independently on #341. The previous commit widened preserve_attempt_worktree to catch OSError, but _reset_would_destroy — which runs immediately after that fault, against the same git binary — still guarded verify.GitError alone. The EMFILE/ENOMEM that broke the capture is exactly what then breaks rev_parse_head/attempt_dirty, so the errno escaped the probe and crashed the rollback anyway. The asymmetry between the sibling handlers was the defect, not either half. Also collapse the duplicated snapshot-failure stub onto _fail_snapshot.
…ause Third link in the same chain, found by Greptile on #341. pause_for_manual_ recovery probes commits_above to choose its notice shape, guarded by a comment saying a git fault there must not block the pause itself — but it caught only verify.GitError, so an untranslated OSError lost the pause the caller had already decided to take. That probe is likelier to hit EMFILE on the new snapshot_failed path than anywhere else: the fault that broke the capture is still in force when we come to write the notice. Degrading to "no commits" costs only notice shape; the crash cost the pause.
|
Third finding confirmed and fixed in Valid, and the sharpest of the three: that probe's own comment already declared the contract ("advisory probe: a git fault here must not block the pause itself") and the code did not honour it for Covered by Two process notes, because both nearly shipped a false green:
Worth flagging for anyone else ablation-testing here — a size-preserving ablation plus sub-second writes is a silent false-negative generator, and it fails in the safe-looking direction. On the pattern rather than the instance: three rounds found the same defect one frame further along the same chain, which says the per-site defence isn't the right shape. |
snapshot_worktree's docstring still described the best-effort "journal and proceed" contract that #340 reversed, contradicting preserve_attempt_worktree in the same change. It also promised GitError on any failure, though _run_git translates only a timeout and the TemporaryDirectory can raise OSError before any git child is spawned. Also qualify "both paths now refuse on the same terms": the symmetry is in the refusal policy, not the guard — preserve_attempt_commits still catches GitError alone, so a spawn fault there crashes instead of refusing (#343).
preserve_attempt_commits read the range above baseline with no try at all, so an ordinary git timeout — which _run_git does translate, and which every sibling on this path already treats as routine — crashed the run mid-rollback. A spawn-level OSError did the same there and at the ref write, and at the rollback's dirty check one frame earlier. An un-determinable range now takes the preservation-failure path rather than the `not commits` early return, which would have let the reset run blind: it refuses under allow_pause and journals on a re-drive, matching the contract an un-parked ref already followed. Journalled as attempt-preserve-enumerate-failed so a post-mortem can distinguish it from a ref that would not take — only the latter can report a HEAD. These are the four frames upstream of the three #341 already covered; the repo-wide question of translating OSError at the _run_git chokepoint stays open in #343. Tests parametrize over exception *factories*: shared instances are built once at collection and re-raising mutates __traceback__, which under pytest-randomly made the cases couple and fail for the wrong reason.
The last OSError-blind guard in the collaborator. apply_patch reads the patch from disk, so an ENOENT/EACCES/ENOSPC arrives untyped and crashed straight past the escalation this branch exists to perform — leaving a half-restored tree with no attention file, on the dispatch path rather than the rollback one. recovery_flow.py is now uniformly OSError-safe; the remaining 19 blind guards across 8 other modules stay with the chokepoint question in #343.
1 participant
Closes #340.
The decision
#340 asked whether a failed
snapshot_worktreeshould block the rollback reset, and recorded three arguments against. Validating them turned up that one is factually wrong, and that the severity is inverted.rollback_or_pause's auto-recover arm runspreserve_attempt_commits→preserve_attempt_worktree→safe_reset, with the reset unconditional and outside anytry. The commits step refuses to reset past work it could not park; the worktree step journaled and let the reset run. So the path that did pause guarded the more recoverable half — orphaned commits stay in the object store, reachable by reflog/git fsckuntilgc— while the path that did not pause guarded work that areset --harddestroys permanently.The issue's counter-argument that this "collides with #161's invariant that an in-worktree rollback never pauses" does not hold:
preserve_attempt_commitsreceivesallow_pause=not redrive, so a plain in-worktree dev retry already pauses on a commits-preserve failure, andpause_for_manual_recovery's own comment documents that case and namesworkspace.rootin every emitted command. The real #161 invariant is only thatscm.rollback_on_failuredoes not gate in-worktree recovery, and it is untouched here — both its tests stay green unmodified.Two corrections to the issue text while validating: the raise surface is nine git spawns, not seven (it misses
rev_parse_headanduntracked_files), each of which can also fail via_run_git's timeout translation. And this hazard has already been patched once by cause —verify.pysynthesizes a git identity precisely so that failure mode can't "reset past the very work this ref exists to preserve", leaving eight other causes open.What changed
1.
preserve_attempt_worktreerefuses the reset on the same terms as its sibling. It takes the same requiredallow_pause, and on a capture failure journals, latchespreserve_partial, then pauses — unless a_reset_would_destroyprobe says nothing unparked is at stake.The probe reuses the existing
verify.attempt_dirty, called against HEAD rather than the attempt baseline. That one substitution makes it report exactly the tracked edits and run-created untracked filessafe_rollbackis about to drop, while ignoring commits, which the previous step has already parked or paused on. So a capture failure over a tree whose content was all committed still resets — a git fault must not halt an unattended run over a harmless reset (cf. #123). It fails safe on its ownGitError, mirroring the dirty check's doctrine from #156.New notice shape (d) offers a git-free rescue (copy the files out) rather than a git-based one, since whatever broke the snapshot may still be breaking git, and points at the
attempt-worktree-preserve-failedjournal entry for the cause.Blast radius is narrow. The new pause needs
(in_unit_worktree or rollback_on_failure) and not redrive. Under the shipped defaults (isolation="none",rollback_on_failure=false) the snapshot is reached only viacause="resolved", which forcesallow_pause=False— so this is inert on default config. It bitesrollback_on_failure = trueand in-worktree dev retries.#338 is narrowed, not superseded. The re-drive path still journals-and-resets, so
preserve_partialand the commits-only defer notice are still load-bearing there. Its regression test moves tocause="resolved"accordingly.2.
safe_rollbackfails loud when it cannot take its preserve snapshot. A failedgit stash createwas swallowed into an empty snapshot, which silently disabled the entirepreserverestore — so the hard reset reverted exactly the paths the caller asked to keep (a resolved re-drive's corrected spec) with no error anywhere. Now raises before the reset, scoped to callers that requested a restore so both sweep sites keep degrading.3. A spawn-level
OSErrorduring the snapshot degrades instead of crashing._run_gittranslates only a timeout, so an EMFILE/ENOMEM escaped untyped out of the middle of a rollback. Preservation is observation, so it joins the journal-and-decide path;safe_resetis the repair write and still raises.Three separate commits so each can be judged independently.
Verification
trunk check(no path filter): cleanAll 10 ablations were run and each made its named test fail. Three are inverse ablations, which matter because the assertions they guard are negative and would otherwise pass vacuously:
..._proceeds_when_nothing_would_be_lostFalseinstead of failing safe..._probe_fault_pauses..._pause_names_the_unit_worktreeexceptback toGitError..._oserror_degrades_into_the_typed_pathpreserve_partiallatch (#338)preserve_partialout of theexcept(#338)..._dirty_preserve_ref_wins_and_subsumes......_raises_when_the_preserve_snapshot_cannot_be_takenand preservescope on that raise..._degrades_stash_failure_when_nothing_to_preservetest_rollback_in_unit_worktree_auto_recovers_even_when_offandtest_dev_retry_in_worktree_auto_recovers_instead_of_pausing(the #161 pins) are unmodified and green.Not fixed here
_deferadvances the task toPhase.DEFERREDbefore calling_rollback_or_pause, andDEFERREDis terminal, so a pause there permanently skips thestory-deferredjournal entry and the defer notification. Pre-existing — reachable today via the commits-preserve pause — and this change widens it to a second path. Impact is forensics only: the pause leaves the tree untouched, so the deferred-work restore has nothing to restore, and the operator gets a louder ACTION REQUIRED notice instead. Filed separately.Summary by CodeRabbit